-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-43218: [C++] Resolve Abseil like any other dependency in the build system #43219
Conversation
And don't pass a pkg-config parameter for absl as there isn't a single package but many. Co-authored-by: Sutou Kouhei <[email protected]>
@github-actions crossbow submit -g cpp |
Revision: c43af58 Submitted crossbow builds: ursacomputing/crossbow @ actions-402e522d16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove absl_SOURCE=BUNDLED
from Fedora 39's Dockerfile?
diff --git a/ci/docker/fedora-39-cpp.dockerfile b/ci/docker/fedora-39-cpp.dockerfile
index 8ecaa6c3ca..33d1182309 100644
--- a/ci/docker/fedora-39-cpp.dockerfile
+++ b/ci/docker/fedora-39-cpp.dockerfile
@@ -77,8 +77,7 @@ RUN /arrow/ci/scripts/install_sccache.sh unknown-linux-musl /usr/local/bin
# PYARROW_TEST_GANDIVA=OFF: GH-39695: We need to make LLVM symbols visible in
# Python process explicitly if we use LLVM 17 or later.
-ENV absl_SOURCE=BUNDLED \
- ARROW_ACERO=ON \
+ENV ARROW_ACERO=ON \
ARROW_AZURE=OFF \
ARROW_BUILD_TESTS=ON \
ARROW_DEPENDENCY_SOURCE=SYSTEM \
We may also need this to avoid an unexpected combination:
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index a973c47848..1d382c967f 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -4154,6 +4154,11 @@ if(ARROW_WITH_GRPC)
endif()
set(ARROW_GRPC_REQUIRED_VERSION "1.30.0")
+ if(absl_SOURCE STREQUAL "BUNDLED" AND NOT gRPC_SOURCE STREQUAL "BUNDLED")
+ # System gRPC can't be used with bundled Abseil
+ message(STATUS "Forcing gRPC_SOURCE to BUNDLED because absl_SOURCE is BUNDLED")
+ set(gRPC_SOURCE "BUNDLED")
+ endif()
if(NOT Protobuf_SOURCE STREQUAL gRPC_SOURCE)
# ARROW-15495: Protobuf/gRPC must come from the same source
message(STATUS "Forcing gRPC_SOURCE to Protobuf_SOURCE (${Protobuf_SOURCE})")
@github-actions crossbow submit -g cpp |
Revision: 164898f Submitted crossbow builds: ursacomputing/crossbow @ actions-0dab8bdfd4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit e65c1e2. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
The workarounds around Abseil resolution don't seem necessary anymore and they don't work on all possible configurations of the build.
What changes are included in this PR?
Removal of the
ensure_absl
macro and adding a call toresolve_dependency
when depending on the Google Cloud SDK (a GCS filesystem dependency) or gRPC (a flight dependency).Are these changes tested?
Yes, by me trying different build configurations on my macOS and existing builds in CI.